Skip to content

bugfix(aigroup): Prevent game crash when a player is selected in Replay playback (2)#2711

Open
Caball009 wants to merge 4 commits into
TheSuperHackers:mainfrom
Caball009:fix_object_selection_replay_crash
Open

bugfix(aigroup): Prevent game crash when a player is selected in Replay playback (2)#2711
Caball009 wants to merge 4 commits into
TheSuperHackers:mainfrom
Caball009:fix_object_selection_replay_crash

Conversation

@Caball009
Copy link
Copy Markdown

@Caball009 Caball009 commented May 14, 2026

With the fix from #1212 AIGroupPtr currentlySelectedGroup would be accessed even if it had been destroyed and its memory deallocated. This is a use-after-free bug. It relies on the memory not being overwritten on deallocation.

If macro MEMORYPOOL_DEBUG (enabled by RTS_DEBUG) is enabled, freed memory is overwritten with a garbage value, so the game crashes when currentlySelectedGroup is dereferenced (at line 2132).

TODO:

  • Tweak TSH comment.
  • Replicate in Generals.

@Caball009 Caball009 added Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Debug Is mostly debug functionality Stability Concerns stability of the runtime Crash This is a crash, very bad labels May 14, 2026
@Caball009 Caball009 force-pushed the fix_object_selection_replay_crash branch from 29b2761 to 2909a9e Compare May 14, 2026 19:03
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR is a follow-up to #1212 that replaces a use-after-free heuristic (getCount() == 0 on a potentially-freed AIGroup) with a proper pointer-liveness check (doesGroupExist) that scans m_groupList by address without ever dereferencing the suspect pointer.

  • GameLogicDispatch.cpp: The #if RETAIL_COMPATIBLE_AIGROUP guard now calls TheAI->doesGroupExist(currentlySelectedGroup) instead of reading getCount() through a possibly-freed pointer; inside this guard AIGroupPtr is AIGroup*, so the call compiles cleanly.
  • Generals and GeneralsMD headers/implementations: doesGroupExist(AIGroup* group) const is added to both builds, resolving the compilation gap that would have occurred if only one target had the declaration.
  • The implementation reuses the same std::find + m_groupList pattern already present in destroyGroup, so no new headers or idioms are introduced.

Confidence Score: 5/5

Safe to merge — the fix is minimal, well-scoped, and correctly addresses the use-after-free without introducing new patterns or dependencies

The change replaces a crash-prone dereference of a freed pointer with a pointer-value scan that never dereferences the suspect address. Both Generals and GeneralsMD receive matching declarations and implementations, and the doesGroupExist implementation follows the same std::find + m_groupList idiom already used by destroyGroup in the same file

No files require special attention

Important Files Changed

Filename Overview
Core/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp Replaces the use-after-free getCount() == 0 heuristic with a safe doesGroupExist pointer-liveness check; updates the comment attribution and date
Generals/Code/GameEngine/Include/GameLogic/AI.h Adds doesGroupExist(AIGroup* group) const declaration to the Generals AI class, resolving the missing-declaration issue from PR #1212's follow-up
Generals/Code/GameEngine/Source/GameLogic/AI/AI.cpp Implements doesGroupExist using std::find over m_groupList; consistent with the existing destroyGroup pattern that already uses std::find on the same list
GeneralsMD/Code/GameEngine/Include/GameLogic/AI.h Mirrors the Generals header change, adding doesGroupExist declaration to the Zero Hour AI class
GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AI.cpp Mirrors the Generals implementation; identical doesGroupExist body using std::find over m_groupList

Sequence Diagram

sequenceDiagram
    participant GLD as GameLogicDispatch
    participant AI as TheAI (AI singleton)
    participant GL as m_groupList

    GLD->>GLD: "currentlySelectedGroup != nullptr?"
    alt pointer non-null
        GLD->>AI: doesGroupExist(currentlySelectedGroup)
        AI->>GL: std::find(begin, end, group)
        GL-->>AI: iterator (found / end)
        AI-->>GLD: true (exists) / false (destroyed)
        alt group not in list (destroyed)
            GLD->>GLD: return early — avoid use-after-free
        else group still alive
            GLD->>GLD: proceed to process selection
        end
    end
Loading

Reviews (3): Last reviewed commit: "Made 'AI::doesGroupExist' member functio..." | Re-trigger Greptile

Comment thread Core/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp
Comment thread Core/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp Outdated
Copy link
Copy Markdown

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I have not thought of that.

@xezon xezon added Major Severity: Minor < Major < Critical < Blocker and removed Minor Severity: Minor < Major < Critical < Blocker labels May 15, 2026
@xezon xezon added this to the Major bug fixes milestone May 15, 2026
@Caball009
Copy link
Copy Markdown
Author

Replicated in Generals.

Comment thread GeneralsMD/Code/GameEngine/Include/GameLogic/AI.h Outdated
Copy link
Copy Markdown

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Crash This is a crash, very bad Debug Is mostly debug functionality Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants